Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use SO_REUSEPORT_LB on FreeBSD #106

Merged
merged 6 commits into from
Aug 17, 2023

Conversation

chaitanyaprem
Copy link
Contributor

Added new socket option supported in freeBSD systems as per #105

Tested in macOS only. Needs to be tested on a freeBSD system to actually verify if this option is working fine or not.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this on an actual FreeBSD?

control_unix.go Outdated Show resolved Hide resolved
control_unix.go Outdated
if err != nil {
return
}
err = unix.SetsockoptInt(int(fd), unix.SOL_SOCKET, FREEBSD_SO_REUSEPORT_LB, 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be executed on FreeBSD. We can't just set a random sock opt on other platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I was thinking of making it future proof by having it for all unix systems.
But if these macros are defined per OS, then it makes sense to protect it as per runtime.
Will change this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constant is only defined on FreeBSD. You'll need to use build flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, tested on freeBSD and macOS after these changes.

@chaitanyaprem
Copy link
Contributor Author

chaitanyaprem commented Aug 16, 2023

Have you tested this on an actual FreeBSD?

Installed a freeBSD VM and tested on it after addressing review comments.

@marten-seemann marten-seemann changed the title feat: Add support for SO_REUSEPORT_LB option as per freeBSD use SO_REUSEPORT_LB on FreeBSD Aug 16, 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the changes except for the change in build flags are needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah..right, Thanks for your patience man.
Needed a lot of handholding for this one .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. The fact that I have to approve CI every time makes things more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one thing i wanted to find out, can't this be left to run automatically?
Would save your time and turn-around time for public contributors as well, because if there are any issues we can fix them based on CI failures itself.

@marten-seemann
Copy link
Contributor

gofmt is unhappy.

@chaitanyaprem
Copy link
Contributor Author

chaitanyaprem commented Aug 17, 2023

gofmt is unhappy.

Ah, one of those times i used vim and not vscode.
Will address this.
Maybe this can be added as a pre-commit hook?

@marten-seemann marten-seemann merged commit 45f5681 into libp2p:master Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants